Skip to content

Implement file read/write/seek in Windows #4275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 22, 2025
Merged

Implement file read/write/seek in Windows #4275

merged 2 commits into from
May 22, 2025

Conversation

CraftSpider
Copy link
Contributor

I can split seek off if desired, though that means I can't enable the whole file read/write shim test in fs.rs in one go

@CraftSpider
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Apr 20, 2025
RalfJung
RalfJung reviewed Apr 29, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, thanks! Just some minor comments. Nice to see that the blocking read/write API works for the Windows functions as well.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@RalfJung
Copy link
Member

@CraftSpider what's the status of this? A standard library test started using NtReadFile directly, and I think this PR would let us run that test (rather than having to disable it in Miri which I will do now). :)

@RalfJung
Copy link
Member

RalfJung commented May 18, 2025 •
edited
Loading

There's a FIXME added in rust-lang/miri-test-libstd#96 that we can hopefully remove when this lands.

@CraftSpider
Copy link
Contributor Author

I'll try to get this updated soon, I've just been doing other stuff - hopefully later today or tomorrow.

@rustbot

This comment has been minimized.

Sign in to view
@CraftSpider
Copy link
Contributor Author

(modulo tests passing)
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 19, 2025
RalfJung
RalfJung reviewed May 20, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it seems there's one pretty big issue, see the comments regarding when the callback is called...
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 20, 2025
@RalfJung
Copy link
Member

I assume this is
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 21, 2025
RalfJung
RalfJung reviewed May 21, 2025
View reviewed changes
@RalfJung
Copy link
Member

This looks great, thanks! After fixing the last nits, please squash the commits using ./miri squash. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 21, 2025
@CraftSpider
Copy link
Contributor Author

miri squash is failing locally on my Windows machine, I'll try to fix it tonight...

@RalfJung
Copy link
Member

Ah, bummer... you can do a manual squash instead, this was just meant to make things easier.

@RalfJung
Copy link
Member

I assume you meant to
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 22, 2025
@RalfJung RalfJung added this pull request to the merge queue May 22, 2025
@RalfJung RalfJung removed this pull request from the merge queue due to a manual request May 22, 2025
@RalfJung RalfJung enabled auto-merge May 22, 2025 07:11
@RalfJung
Copy link
Member

Nice, reading from stdin should also finally work with this. :)

@RalfJung RalfJung added this pull request to the merge queue May 22, 2025
Merged via the queue into rust-lang:master with commit 6e01f6e May 22, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CraftSpider @rustbot @RalfJung